Additional outputs to longitudinal functional workflow Fix #190#282
Additional outputs to longitudinal functional workflow Fix #190#282jpillai00 wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Extends the longitudinal functional workflow to re-run nuisance regression in longitudinal template space using the raw (non-bandpass-filtered) nuisance regressors produced during cross-sectional preprocessing, while also exporting additional regressor artifacts from the cross-sectional stage.
Changes:
- Expand functional output structures to carry both raw and bandpass-filtered regressor
.1Dfiles, and add longitudinal-spaceregressed_bold/cleaned_boldoutputs per regressor strategy. - Update longitudinal orchestration/BIDS resolve+export to accept a regressor strategy list and to resolve raw regressor files per strategy.
- Add
--regressorsupport to the longitudinal CLI and update unit tests accordingly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/orchestration/test_longitudinal.py | Updates mocks and process_func calls to pass regressors; adjusts/removed tests impacted by new required inputs. |
| tests/unit/cli/test_longitudinal.py | Adds regressor to the longitudinal CLI test namespace defaults. |
| tests/unit/bids/test_exports.py | Updates expected derivative file counts due to new exported regressor artifact(s). |
| src/rbc/workflows/functional.py | Adds raw+filtered regressor outputs to cross-sectional outputs; adds longitudinal-space regression outputs and implements re-regression in longitudinal space. |
| src/rbc/orchestration/longitudinal.py | Threads regressor strategies through longitudinal functional processing; adds default regressors parameter. |
| src/rbc/cli/longitudinal.py | Adds --regressor CLI option and passes it through to orchestration. |
| src/rbc/bids/longitudinal.py | Resolves raw regressor .1D files per strategy and exports longitudinal-space regressed/cleaned BOLD outputs. |
| src/rbc/bids/functional.py | Exports an additional bandpass-filtered regressor .1D artifact per regressor strategy. |
Comments suppressed due to low confidence (1)
tests/unit/orchestration/test_longitudinal.py:346
test_missing_required_file_raisesno longer covers the newly-required longitudinal inputs (bold_maskand the per-regressorregressor_files). Sinceresolve_longitudinal_func()now usesexpect()for these, it would be good to extend the parametrization to assert aFileNotFoundErrorwhen the mask or a regressor.1Dfile is missing (per regressor strategy).
@pytest.mark.parametrize(
("match_field", "match_kwargs"),
[
("bold", {"suffix": "bold", "desc": "preproc"}),
("sbref", {"suffix": "sbref"}),
(
"bold_to_anat_xfm",
{
"suffix": "xfm",
"desc": "linear",
"extension": ".txt",
"extra": {"from": "bold", "to": "T1w", "mode": "image"},
},
),
],
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Largely looks good to me, a couple of minor changes with the type hints and casting. Also looks like it needs a rebase now.
One thing that comes to mind since the longitudinal currently relies on flags to denote the modality to run (--anatomical, --functional), should there be an early check to ensure the anatomical has been run? It should error out pretty early none the less since the data is relied upon on in the first step.
ce86168 to
657623e
Compare
657623e to
d4fa651
Compare
|
Thanks @jpillai00 — the Closed as part of #301 Stage 0. |
regressor_file now carries the raw (unfiltered) regressor .1D files as computed from native-space BOLD. The new bpf_regressor_file field holds the bandpass-filtered version that 3dTproject actually applied, exported with desc-<strategy>Filtered for provenance. This lets the longitudinal pipeline reuse the raw regressors without recomputation, matching the cross-sectional principle: one regressor computation per run, applied in each target space. Port of the regressor-split idea from #282. Co-authored-by: Janhavi Pillai <janhaviiyengar@gmail.com>
FunctionalLongOutputs gains regressed_bold and cleaned_bold dicts keyed by regressor strategy. longitudinal_process now accepts regressor_files (raw .1D from cross-sectional) and re-runs apply_regression + apply_regression_bandpass on the warped BOLD. No regressor recomputation -- same matrix, different target space. bold_mask is now mandatory (was Path | None). BIDS export writes per-regressor desc-regressed and desc-preproc BOLD with reg-<strategy> entity. resolve_longitudinal_func resolves the raw regressor files per strategy. CLI gains --regressor with the same choices as cross-sectional (36-parameter, aCompCor). Port of the longitudinal regression logic from #282. Co-authored-by: Janhavi Pillai <janhaviiyengar@gmail.com>
regressor_file now carries the raw (unfiltered) regressor .1D files as computed from native-space BOLD. The new bpf_regressor_file field holds the bandpass-filtered version that 3dTproject actually applied, exported with desc-<strategy>Filtered for provenance. This lets the longitudinal pipeline reuse the raw regressors without recomputation, matching the cross-sectional principle: one regressor computation per run, applied in each target space. Port of the regressor-split idea from #282. Co-authored-by: Janhavi Pillai <janhavi.pillai@gmail.com>
FunctionalLongOutputs gains regressed_bold and cleaned_bold dicts keyed by regressor strategy. longitudinal_process now accepts regressor_files (raw .1D from cross-sectional) and re-runs apply_regression + apply_regression_bandpass on the warped BOLD. No regressor recomputation -- same matrix, different target space. bold_mask is now mandatory (was Path | None). BIDS export writes per-regressor desc-regressed and desc-preproc BOLD with reg-<strategy> entity. resolve_longitudinal_func resolves the raw regressor files per strategy. CLI gains --regressor with the same choices as cross-sectional (36-parameter, aCompCor). Port of the longitudinal regression logic from #282. Co-authored-by: Janhavi Pillai <janhavi.pillai@gmail.com>
* Split FunctionalOutputs.regressor_file into raw + bandpass-filtered regressor_file now carries the raw (unfiltered) regressor .1D files as computed from native-space BOLD. The new bpf_regressor_file field holds the bandpass-filtered version that 3dTproject actually applied, exported with desc-<strategy>Filtered for provenance. This lets the longitudinal pipeline reuse the raw regressors without recomputation, matching the cross-sectional principle: one regressor computation per run, applied in each target space. Port of the regressor-split idea from #282. Co-authored-by: Janhavi Pillai <janhavi.pillai@gmail.com> * Add per-regressor regression to longitudinal functional workflow FunctionalLongOutputs gains regressed_bold and cleaned_bold dicts keyed by regressor strategy. longitudinal_process now accepts regressor_files (raw .1D from cross-sectional) and re-runs apply_regression + apply_regression_bandpass on the warped BOLD. No regressor recomputation -- same matrix, different target space. bold_mask is now mandatory (was Path | None). BIDS export writes per-regressor desc-regressed and desc-preproc BOLD with reg-<strategy> entity. resolve_longitudinal_func resolves the raw regressor files per strategy. CLI gains --regressor with the same choices as cross-sectional (36-parameter, aCompCor). Port of the longitudinal regression logic from #282. Co-authored-by: Janhavi Pillai <janhavi.pillai@gmail.com> * Add tests for longitudinal functional regression (Stage 5 of #301) Unit tests: resolve_longitudinal_func with single/multiple regressors, missing regressor raises, mandatory bold_mask; export file counts and reg-<strategy> entity validation. Tier-2 integration: apply_regression and apply_regression_bandpass on warped BOLD with raw regressors produce non-degenerate outputs. Tier-4 full_pipeline: longitudinal functional outputs exist, regressed and cleaned BOLD have non-zero variance, bold mask is binary. * Remove redundant regressor_set, replace fake tests with real ds000114 integration - Drop regressor_set param from longitudinal_process; iterate regressor_files.keys() instead. The resolve layer already filters to the requested strategies, so the dict keys ARE the set. Eliminates the caller sync burden. - Delete tests/full_pipeline/longitudinal/ -- the conftest hacked the cross-sectional anat brain as a "longitudinal template", which exercises the code path but not the actual transform chain. - Rewrite tests/integration/longitudinal/test_regression_reuse.py as a proper end-to-end CLI test: rbc functional -> rbc longitudinal functional on ds000114 sub-01 ses-test via subprocess. Asserts expected BIDS tree, non-degenerate variance, and binary mask. - Add ds000114_func_derivatives and longitudinal_func_output fixtures to the integration conftest so the full cross-sectional -> longitudinal chain runs on real multi-session data with docker. * Fix integration test fixture ordering to avoid bids2table discovery issue ds000114_func_derivatives now depends on ds000114_anat_derivatives directly instead of longitudinal_template_output. This ensures rbc functional runs before the template step writes ses-longitudinal files into the derivatives dir, avoiding a potential bids2table discovery issue where the ses-longitudinal anat files interfere with the functional pipeline's anat resolution. longitudinal_func_output depends on both ds000114_func_derivatives and longitudinal_template_output to ensure both cross-sectional functional outputs and the longitudinal template are present before rbc longitudinal functional runs. * Debug: log derivatives tree before rbc functional in integration fixture * Debug: dump bids2table output for derivatives dir before rbc functional * Debug: inline rbc functional call with verbose + tree dump on failure * Fix integration fixture: --task filter drops anat rows Filters.apply() applies the --task filter to ALL rows including anat. Anat files have task=null, so --task fingerfootlips drops them, causing resolve_functional to fail with FileNotFoundError on desc-brain_T1w. Drop --task from both rbc functional and rbc longitudinal functional fixture calls. ds000114 only has one task per session so the filter isn't needed. Remove debug logging from previous commits. * Fix Filters.apply() task filter dropping anat rows The --task filter applied pl.col("task") == value to all rows, but anat/dwi files have task=null, so they were silently dropped. This caused rbc functional --task <label> to fail with FileNotFoundError on desc-brain_T1w for any dataset. Fix: use pl.col("task").is_null() | (pl.col("task") == value) so rows without a task entity (anat, dwi, fmap) pass through. Affects functional and all orchestration; metrics/qc pre-filter to datatype=func so they were never hit. Restore --task in integration fixtures now that the fix is in place. * Fix expected filenames: reg entity precedes desc in BIDS builder output --------- Co-authored-by: Janhavi Pillai <janhavi.pillai@gmail.com>
The longitudinal functional workflow previously only warped cross-sectional outputs (sbref, preproc BOLD, brain mask, and the BOLD-to-longitudinal transform) into longitudinal template space. This PR extends it to recompute nuisance regression in longitudinal space using the raw (non-bandpass-filtered) regressors from cross-sectional preprocessing.
Key changes:
FunctionalLongOutputsnow includesregressed_boldandcleaned_boldasdict[str, Path], supporting multiple regressor strategieslongitudinal_processacceptsregressor_files: dict[str, Path]and re-runs both non-bandpassed and bandpassed regression in longitudinal spaceFunctionalOutputsseparately exportsregressor_file(raw, passed to longitudinal workflow) andbpf_regressor_file(bandpass-filtered) - in cross sectionalresolve_longitudinal_funcresolves raw regressor files per regressor strategyexport_longitudinal_funcsaves per-regressorregressed_boldandcleaned_bold--regressorargument added to the longitudinal CLI